3d tiles terrain #12963
3d tiles terrain #12963
Conversation
…ior that resolves values immediately
3D Tiles terrain water mask
There was a problem hiding this comment.
Thanks @jjhembd, overall this looks pretty good to me!
I mostly reviewed this from a docs perspective. I believe most of this has already been verified and sitting around for a while. I also tested the sandcastle some but probably not exhaustive. I think it could be good to have a second perspective on the actual logic
| options = options ?? Frozen.EMPTY_OBJECT; | ||
|
|
||
| //>>includeStart('debug', pragmas.debug); | ||
| if (!defined(options) || !defined(options.buffer)) { | ||
| throw new DeveloperError("options.buffer is required."); | ||
| } | ||
| if (!defined(options.width)) { | ||
| throw new DeveloperError("options.width is required."); | ||
| } | ||
| if (!defined(options.height)) { | ||
| throw new DeveloperError("options.height is required."); | ||
| } | ||
| Check.defined("options.buffer", options.buffer); |
There was a problem hiding this comment.
Edit: NO change requested right now, just commenting for discussion. Probably best as it's own issue
I think this is equivalent to:
//>>includeStart('debug', pragmas.debug);
Check.defined("options", options);
Check.defined("options.buffer", options.buffer);Or maybe even better Check.typeOf.object("options", options)
I started to call this out in other places but realized we actually use this pattern a lot and it's probably not worth addressing in this PR.
Specifically I think it's an issue when options is not marked as optional but the ?? makes it behave as if it is. Using this will punt potential errors down the call chain to somewhere where it can't work with undefined. I think it would be better if this function itself throws errors because it can't access values of undefined.
| * @param {number} options.skirtHeight The height of the skirt to add on the edges of the tile. | ||
| * @param {Credit[]} [options.credits] Array of credits for this tile. | ||
| */ | ||
| function Cesium3DTilesUpsampleTerrainData(options) { |
There was a problem hiding this comment.
Should this class be in it's own file? no strong preference I think, just asking
There was a problem hiding this comment.
Cesium3DTilesUpsampleTerrainData and its "parent" Cesium3DTilesTerrainData share the helper functions interpolateMeshHeight and upsampleMesh.
To separate the classes while avoiding circular dependencies, I suppose we could make those helpers static methods of Cesium3DTilesUpsampleTerrainData. This structure makes sense in the case of upsampleMesh, which always returns a Cesium3DTilesUpsampleTerrainData, but feels a little forced in the case of interpolateMeshHeight.
The other issue is that both the helpers have very long function signatures, which is tolerable (maybe?) for an internal helper, but doesn't make for a very pretty (or maintainable or testable) API for a class method.
I don't love the current structure but I think a refactor might be too much for this PR.
| const terrainProvider = | ||
| await Cesium.Cesium3DTilesTerrainProvider.fromIonAssetId(3923568, { | ||
| requestVertexNormals: true, // Needed for hillshade lighting | ||
| requestWaterMask: true, // Needed to distinguish land from water | ||
| }); |
There was a problem hiding this comment.
This sandcastle essentially has only 1 change compared to the "Globe materials - Water mask" sandcastle. That's the terrain provider. Would it be better to combine them and have a dropdown or something to toggle which terrain provider is being used? Or is there a better, more unique, way to demonstrate that this is using 3D Tiles as terrain?
There was a problem hiding this comment.
I updated the description in the .yaml file to emphasize the 3D Tiles as terrain aspect.
I agree there's still a lot of overlap. I can combine them if you prefer, though I think I would set the 3D Tiles data as the starting point, just to make sure we capture that dataset & code path during end-to-end testing or manual release checks.
| _y, | ||
| _level, | ||
| ) { | ||
| return undefined; |
There was a problem hiding this comment.
Is this function supposed to be "unimplemented"?
There was a problem hiding this comment.
Good question... Only CesiumTerrainProvider implements this function.
EllipsoidTerrainProvider, VRTheWorldTerrainProvider, GoogleEarthEnterpriseTerrainProvider, and ArcGISTiledElevationTerrainProvider all return undefined too.
I haven't traced through how it's used, but this looks like an interface feature that isn't absolutely necessary for terrains to work.
|
Thanks for the feedback @jjspace. I think I addressed it where I could, and left comments where the fix is not obvious or maybe out of scope. Let me know if I'm missing anything, or if there's a good way to clean up that I didn't think of. |
Description
This PR adds experimental support for loading 3D Tiles as terrain, via
Cesium3DTilesTerrainProvider.What 3D Tiles data can be used?
3D Tiles loaded with Cesium3DTilesTerrainProvider must follow this structure (thanks @lilleyse for the list):
TILE_MINIMUM_HEIGHT,TILE_MAXIMUM_HEIGHT,TILE_BOUNDING_SPHERE,TILE_HORIZON_OCCLUSION_POINTsemanticsPOSITIONattributeNORMALattribute ifrequestVertexNormalsis trueUNSIGNED_SHORTorUNSIGNED_INTcomponent typerequestWaterMaskis true, primitive must reference anEXT_structural_metadataproperty texture withWATERMASKsemantic.CESIUM_tile_edgesextensionEXT_meshopt_compression/KHR_mesh_quantizationIssue number and link
Resolves #12296
Testing plan
Cesium3DTilesTerrainProviderSpecandCesium3DTilesTerrainDataSpecTODO before moving this out of "Draft":
Checkcalls:EllipsoidalOccluder,GoogleEarthEnterpriseTerrainData,TerrainEncoding(separate from other changes in file),binarySearch,mergeSort,QuantizedMeshTerrainDataSpec. Beware incorrect capitalizations like{Object},{Number[]}! See Clean up docs and type checks for terrain providers #12969ResourceCache, avoid additional URI parsing.Consider dropping the duplicateThe two versions ofImplicitSubtreeCacheinCesium3DTilesTerrainProvider.js(replace withImplicitSubtreeCache.js)ImplicitSubtreeCachehave some key differences, so I think a refactor is out of scope for this PR.TerrainEncodingAuthor checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change